fix: Allow use of relative URLs in config #1754

chlasch · 2021-09-15T21:49:58Z

This PR reverts to previous behavior and allows relative uri-references for self-service URLS. This functionality matches the current documentation and addresses what seems like a bug that was introduced.

Our specific use case is an on-prem application that does not know it's hostname at deployment, and needs to be accessible by arbitrary DNS names or IP hostnames.

Related issue(s)

#1446

Checklist

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

I don't believe a documentation change is needed, as this should be working already according to the config examples and schema. eg. /Dashboard

CLAassistant · 2021-09-15T21:50:01Z

All committers have signed the CLA.

codecov · 2021-09-15T22:32:44Z

Codecov Report

Merging #1754 (b24e9a3) into master (02c9e26) will decrease coverage by 1.19%.
The diff coverage is 88.00%.

❗ Current head b24e9a3 differs from pull request most recent head 2d1d017. Consider uploading reports for the commit 2d1d017 to get more accurate results

@@            Coverage Diff             @@
##           master    #1754      +/-   ##
==========================================
- Coverage   75.13%   73.93%   -1.20%     
==========================================
  Files         292      260      -32     
  Lines       15125    12715    -2410     
==========================================
- Hits        11364     9401    -1963     
+ Misses       2950     2685     -265     
+ Partials      811      629     -182

Impacted Files	Coverage Δ
driver/config/config.go	`81.53% <78.57%> (-0.16%)`	⬇️
internal/testhelpers/selfservice.go	`100.00% <100.00%> (ø)`
text/message_system.go	`0.00% <0.00%> (-100.00%)`	⬇️
cmd/migrate/sql.go	`57.14% <0.00%> (-42.86%)`	⬇️
text/message_recovery.go	`63.04% <0.00%> (-36.96%)`	⬇️
text/message_validation.go	`64.70% <0.00%> (-35.30%)`	⬇️
cmd/cliclient/migrate.go	`0.00% <0.00%> (-34.55%)`	⬇️
ui/node/attributes.go	`27.58% <0.00%> (-33.29%)`	⬇️
text/message_verification.go	`67.56% <0.00%> (-32.44%)`	⬇️
text/message_login.go	`71.69% <0.00%> (-28.31%)`	⬇️
... and 117 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 034806f...2d1d017. Read the comment docs.

zepatrik

To prevent regression of this bug (again 😅), it would be good to add a case to one of the handlers that ensures it actually does a relative redirect when configured to do so.
Ideally even in all flow handlers like https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/login/handler_test.go
https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/registration/handler_test.go
and the others.

driver/config/config_test.go

aeneasr

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

driver/config/config.go

chlasch · 2021-09-16T19:00:21Z

To prevent regression of this bug (again 😅), it would be good to add a case to one of the handlers that ensures it actually does a relative redirect when configured to do so.
Ideally even in all flow handlers like https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/login/handler_test.go
https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/registration/handler_test.go
and the others.

I added an explicit redirect check to the 3 handler tests that already had TestInitFLow functions (login, recovery, registration). I opted for a testhelper function to check the redirect, instead of duplicating the test function in each handler_test, but I'm not sure if this is how you want it done.

zepatrik

Thanks a lot, this looks very good now 🎉

zepatrik · 2021-09-17T08:41:10Z

internal/testhelpers/selfservice.go

@@ -197,3 +197,17 @@ func SelfServiceMakeHookRequest(t *testing.T, ts *httptest.Server, suffix string
 	require.NoError(t, err)
 	return res, string(body)
 }
+
+func GetSelfServiceRedirectLocation(t *testing.T, url string) string {


Perfect 👍

aeneasr

Great work! :)

Could you add tests for the remaining (verification, settings, error) flows also please? :)

That ensures that this never unintentionally breaks - so it’s like an insurance for you :)

chlasch · 2021-09-17T17:20:02Z

Great work! :)

Could you add tests for the remaining (verification, settings, error) flows also please? :)

That ensures that this never unintentionally breaks - so it’s like an insurance for you :)

I can give it a shot, but since those flows don't have the TestInitFlow functions, I got a little stuck in understanding where it should fall within the test.

aeneasr · 2021-09-18T15:22:43Z

Oh I see! If you have difficulties figuring it out feel free to ask here :)

aeneasr · 2021-10-12T05:57:02Z

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

Separate functions for absolute and relative URLS Add distinct test cases for relative urls

Add tests for error, verification, and settings flows

aeneasr

Great job, thank you! :)

aeneasr · 2021-10-27T12:19:16Z

Re-running tests

chlasch requested review from aeneasr and zepatrik as code owners September 15, 2021 21:49

zepatrik requested changes Sep 16, 2021

View reviewed changes

driver/config/config_test.go Outdated Show resolved Hide resolved

aeneasr requested changes Sep 16, 2021

View reviewed changes

driver/config/config.go Show resolved Hide resolved

chlasch force-pushed the fix/relative-urls branch from 1a82ffc to b24e9a3 Compare September 16, 2021 19:44

chlasch requested review from aeneasr and zepatrik September 16, 2021 20:15

zepatrik approved these changes Sep 17, 2021

View reviewed changes

aeneasr reviewed Sep 17, 2021

View reviewed changes

aeneasr marked this pull request as draft October 12, 2021 05:57

chlasch force-pushed the fix/relative-urls branch from b24e9a3 to 2245446 Compare October 22, 2021 12:59

chlasch added 4 commits October 25, 2021 13:16

fix: Allow use of relative URLs in config

6b9eae3

fix: Enhance relative URL handling and tests

8073dc9

Separate functions for absolute and relative URLS Add distinct test cases for relative urls

test: Add deterministic relative redirect tests

5884f77

test: Relative redirect tests for all flows

780ddd2

Add tests for error, verification, and settings flows

chlasch force-pushed the fix/relative-urls branch from 2245446 to 780ddd2 Compare October 25, 2021 17:16

chlasch marked this pull request as ready for review October 25, 2021 17:52

chlasch requested review from zepatrik and aeneasr October 26, 2021 18:04

aeneasr approved these changes Oct 27, 2021

View reviewed changes

Merge branch 'master' into fix/relative-urls

ca8f21a

chore: code review

2d1d017

aeneasr approved these changes Oct 29, 2021

View reviewed changes

aeneasr merged commit 5f73bb0 into ory:master Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Allow use of relative URLs in config #1754

fix: Allow use of relative URLs in config #1754

chlasch commented Sep 15, 2021

CLAassistant commented Sep 15, 2021 •

edited

Loading

codecov bot commented Sep 15, 2021 •

edited

Loading

zepatrik left a comment

aeneasr left a comment

chlasch commented Sep 16, 2021

zepatrik left a comment

zepatrik Sep 17, 2021

aeneasr left a comment

chlasch commented Sep 17, 2021

aeneasr commented Sep 18, 2021

aeneasr commented Oct 12, 2021

aeneasr left a comment

aeneasr commented Oct 27, 2021

fix: Allow use of relative URLs in config #1754

fix: Allow use of relative URLs in config #1754

Conversation

chlasch commented Sep 15, 2021

Related issue(s)

Checklist

Further Comments

CLAassistant commented Sep 15, 2021 • edited Loading

codecov bot commented Sep 15, 2021 • edited Loading

Codecov Report

zepatrik left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

chlasch commented Sep 16, 2021

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik Sep 17, 2021

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

chlasch commented Sep 17, 2021

aeneasr commented Sep 18, 2021

aeneasr commented Oct 12, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented Oct 27, 2021

CLAassistant commented Sep 15, 2021 •

edited

Loading

codecov bot commented Sep 15, 2021 •

edited

Loading